-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/leeds crc new #166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ollietulloch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shilpigoeldev, a few initial comments added
| def populate_raw_field_variables(record) | ||
| @report = record.raw_fields['report'] | ||
| @moltestingtype = record.raw_fields['moleculartestingtype'] | ||
| @value1 = record.raw_fields['proteinimpact'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these @value.. instance variables be given more meaningful names please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ollietulloch ,These variable names are what we are receiving in the file , so are aligned with the rules designed by Fiona and hence I used same names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shilpigoeldev, I can see why you stuck with those names, they just seem a little meaningless wen review code without that context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, logic matches rules for FS and targeted tests. I don't think failing tests are related to these changes?
What?
I have added a new importer for Leeds colorectal new version of files.
How?
By updating importer script and adding new handler.
Testing?
QA counts have been signed off and test added.
Anything Else?
N/A